Add the jj-core crate - #9766
Conversation
9234b2b to
951ed8a
Compare
0b88216 to
ba29d5d
Compare
There was a problem hiding this comment.
Regarding directory structure, are we going to flatten all sub crates that are re-exported from jj-lib?
cli/
core/
lib/
gen-protos/
testutils/ ??
git-backend/
...
or put them under lib/ as before?
cli/
lib/core/
lib/gen-protos/
lib/testutils/
lib/git-backend/
lib/...
Just wondering. I don't have a preference either way.
There was a problem hiding this comment.
I guess this is more a question for @martinvonz. I'd prefer to move to a crates/ directory at some point but Martin said that he prefers the flat structure for now.
There was a problem hiding this comment.
or put them under lib/ as before?
I like this better. Then the crate name could be jj- followed by the path segments joined by -, e.g. jj-cli, jj-lib-core, jj-lib-gen-protos. What do you think?
There was a problem hiding this comment.
IMO, I can do that although I'd prefer if we didn't prefix lib path to the name since these crates should be quite independent of jj-lib.
There was a problem hiding this comment.
I think that actually sounds good to me. I was thinking that things like jj-gen-protos sounded too generic, but gen-protos/ will presumably be split up and go into local-working-copy/gen-protos/, simple-op-store/gen-protos/ etc.
There was a problem hiding this comment.
Looking at this PR again, I was confused why the new crate is under lib/. As you said, jj-core is supposed to be independent of jj-lib, so it seems to me like it shouldn't be under lib/. I think my previous concern was just about gen-protos/ and testutils/, but I think the answer is that we have core/, lib/ and core/testutils/ (if useful) and lib/testutils/. I think the crate names should match the directory path (with an implied jj-). Does that make sense?
There was a problem hiding this comment.
So you still prefer variant 1 of #9766 (comment) if I'm understanding correctly? Then I'll need to move everything back again.
There was a problem hiding this comment.
I think I prefer a mix of them:
cli/
core/
core/testutils/
git-commit-backend/
lib/
lib/testutils/
local-working-copy/
local-working-copy/gen-protos/
simple-op-store/
simple-op-store/gen-protos/
...
Do you follow what I mean? What do you (all) think?
There was a problem hiding this comment.
Do you follow what I mean?
yes, thanks for clarifying.
What do you (all) think?
I still don't have any strong opinion on the structure. And I would guess the same for Yuya but he should speak for himself.
ba29d5d to
ef577d6
Compare
pksunkara
left a comment
There was a problem hiding this comment.
I think str_util, time_util needs to be moved too.
I think the idea is to move most (?) of jj-lib. This is just a first PR. |
|
👍 If we are doing this incrementally and merge this PR first, I will send the PR to move |
ef577d6 to
878837b
Compare
06e5493 to
82455c9
Compare
82455c9 to
950aa50
Compare
| assert_ne!(hash(&42i32), hash(&[42i32][..])); | ||
| } | ||
|
|
||
| // TODO: move this over when we lower `hex_util.rs` |
There was a problem hiding this comment.
why not move hex_util.rs before anything else? it looks like it doesn't depend on anything else
This will be common base for building upon `jj` if you only want the internals which make the other systems work, like the `Backend` or `WorkingCopy` trait. It should be of utmost importance to make the crate as low dependency as possible so its not in the critical path during compilation. Part of #6284
This part of the library is quite basic and it doesn't have too many dependencies which also need to move to make it happen. We also need to move the `ContentHash` macro since all `RepoPath` types depend on it, the macro is moved to a new `jj-core-proc-macros` crate which mirrors the existing structure in `jj-lib`. Since we now have `jj-core-proc-macros` this also deprecates `jj-lib-proc-macros` for external consumers. Part of #6284
As requested by Martin.
This is in preparation of moving `Backend`, `Index` and `Store`. I've also added the `Nothing/EverythingMatcher` even though it's a clear layering violation but I've granted myself an exception for that since they're quite basic. Its a simple move since it only depends on `RepoPathBuf` which already is in the new core crate. Part of #6284
a9a1b10 to
3c50ef1
Compare
This moves the first interface into the new core library. This also moves the `Signer` struct by dropping the `Config` dependency, to preserve the `jj-lib` API it is imported as `CoreSigner`. Part of #6284
Move the pest grammar, the parser and all DSL related parts to the new crate. Its needed so we can move the `WorkspaceName` and `WorkspaceNameBuf` newtypes for the `WorkingCopyStore` trait. See the next patch. Part of #6284
So `jj-core` users also have access to it, since we already did the same thing for the `RevsetParser`. This is part of building a new `jj-core` crate. Part of #6284
Moving this is quite simple and adds another trait to the core crate. Part of #6284
3c50ef1 to
020d847
Compare
There was a problem hiding this comment.
It seems weird to leave behind a repo_path module without RepoPath. I sent #9893 to make this more clean.
These are the simple parts of the
jj-coreextraction, see #6284 (comment) for what this entails.Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.